[Report Scheduler] Documentation of Scheduling Logic #31134
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lpbeliveau-silabs
force-pushed
the
bugfix/report_scheduler_design_comment
branch
from
6d31342 to
aef3e07
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov,
LuDuda,
mhazley and
mkardous-silabs
pullapprove
bot
requested review from
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yufengwangca
|
PR #31134: Size comparison from 19e202e to aef3e07 Increases (31 builds for bl702, cc13x4_26x4, cyw30739, efr32, esp32, k32w, nrfconnect, telink)
Decreases (10 builds for bl602, bl702, bl702l, linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
lpbeliveau-silabs
force-pushed
the
bugfix/report_scheduler_design_comment
branch
from
aef3e07 to
1d1639f
Compare
|
PR #31134: Size comparison from ad98820 to 1d1639f Increases (29 builds for bl702, bl702l, cc13x4_26x4, cyw30739, esp32, k32w, qpg, telink)
Decreases (11 builds for bl602, bl702, bl702l, linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
jmartinez-silabs
approved these changes
Jan 3, 2024
lpbeliveau-silabs
force-pushed
the
bugfix/report_scheduler_design_comment
branch
from
4bbd090 to
60c8876
Compare
lpbeliveau-silabs
force-pushed
the
bugfix/report_scheduler_design_comment
branch
from
60c8876 to
512ffa2
Compare
Co-authored-by: Junior Martinez <[email protected]>
lpbeliveau-silabs
force-pushed
the
bugfix/report_scheduler_design_comment
branch
from
512ffa2 to
03286a0
Compare
|
PR #31134: Size comparison from c7f8ec9 to 03286a0 Increases (30 builds for bl702, cc13x4_26x4, cyw30739, esp32, k32w, nrfconnect, telink)
Decreases (12 builds for bl602, bl702, bl702l, efr32, linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
approved these changes
Jan 4, 2024
| * | ||
| * It inherits the ReadHandler::Observer class to be notified of reportability changes in the ReadHandlers. | ||
| * It inherits the ICDStateObserver class to allow the implementation to generate reports based on the changes in ICD devices state, | ||
| * such as going from idle to active and vice-versa. |
There was a problem hiding this comment.
`such as going from idle to active mode and vice-versa.
| * | ||
| * @brief This class is in charge of determining when a ReadHandler is reportable depending on the monotonic timestamp of the | ||
| * system and the intervals of the ReadHandler. It inherits the TimerContext class to allow it to be used as a context for a | ||
| * TimerDelegate so the TimerDelegate can call the TimerFired method when the timer expires. |
There was a problem hiding this comment.
TimerDelegate so that the TimerDelegate can call the TimerFired method when the timer expires.
| * | ||
| * The Logic to determine if a ReadHandler is reportable at a precise timestamp is as follows: | ||
| * 1: The ReadHandler is in the CanStartReporting state | ||
| * 2: The minimal interval since last report has elapsed |
There was a problem hiding this comment.
The minimal interval since the last report has elapsed
| * The Logic to determine if a ReadHandler is reportable at a precise timestamp is as follows: | ||
| * 1: The ReadHandler is in the CanStartReporting state | ||
| * 2: The minimal interval since last report has elapsed | ||
| * 3: The maximal interval since last report has elapsed or the ReadHandler is dirty |
There was a problem hiding this comment.
The maximal interval since the last report has elapsed or the ReadHandler is dirty
| * | ||
| * @brief This class extends ReportScheduler and provides a scheduling logic for the CHIP Interaction Model Reporting Engine. | ||
| * | ||
| * It is reponsible for implementing the ReadHandler and ICD observers callbacks to the Scheduler can take actions whenever a |
There was a problem hiding this comment.
callbacks to the Scheduler sentence seems off here.
| * un-necessary since the ReadHandler will call MoveToState(HandlerState::CanStartReporting);, which will call | ||
| * OnBecameReportable() and schedule the report. | ||
| * | ||
| * @note This method sets a now Timestamp that is used to calculate the next report timeout. |
There was a problem hiding this comment.
Does a now Timestamp mean set a timestamp to now?
There was a problem hiding this comment.
Yes, it means a timestamp to the moment of the call.
There was a problem hiding this comment.
How about "This method captures the current timestamp, which can later be used to ... "?
| * | ||
| * @brief This class extends ReportSchedulerImpl and overrides it's scheduling logic. | ||
| * | ||
| * It only overrides Observers method where the scheduling logic make it necessary, the others are kept as is. |
| * ## Scheduling Logic | ||
| * | ||
| * This class implements a scheduling logic that aims to make all ReadHandlers report at the same time when possible. | ||
| * The goal is to minimize the different times a device wakes up to report, and thus this aims to schedule all reports at the latest |
There was a problem hiding this comment.
minimize the number of times a device wakes up to report
| * The logic also aims to minimize the impact on the responsivity of the device. | ||
| * | ||
| * The scheduling logic is as follows: | ||
| * - The CalculateNextReportTimeout is called by the same ReadHandler Observer callbacks than the non-synchronized implementation: |
There was a problem hiding this comment.
callbacks as the non-synchronized implementation:
| * * Are Reportable (this prevents a ReadHandler that is not reportable to hold the report of all the others) | ||
| * TODO: Assess if we want to keep this behavior or simply let the min interval be the earliest min interval to prevent cases | ||
| * where a ReadHandler with a dirty path but a very high min interval blocks all reports | ||
| * - If no ReadHandlerNode matches min interval the criteria, the next min interval is set to current timestamp. |
There was a problem hiding this comment.
matches the min interval
| * | ||
| * Additionnal flags have been provided for specific use cases: | ||
| * | ||
| * CanbeSynced: Mechanism to allow the ReadHandler to emit a report if another readHandler is ReportableNow. |
| * Additionnal flags have been provided for specific use cases: | ||
| * | ||
| * CanbeSynced: Mechanism to allow the ReadHandler to emit a report if another readHandler is ReportableNow. | ||
| * This flag can substitute the maximal interval condition or the dirty condition. It is currently only used by the |
There was a problem hiding this comment.
I assume this means "This flag can replace the maximal interval condition or the dirty condition"?
But this is actually really hard to follow. First we say "we are reportable if all three of these things are true", and then we later say "oh, actually, some of those can be false if some of these other things are true".
What this documentation should talk about is conceptually what things might keep us from being reportable and for each one which sets of things might be able to remove that impediment.
As I see, it there are three impediments:
- The ReadHandler is not in CanStartReporting state. The only way to remove this impediment is to be in that state, right?
- The ReadHandler has reported too recently. This can be removed by enough time passing or EngineRunScheduled being set by a timer firing, etc, right?
- The ReadHandler has no need to report. This can be removed by either being dirty (hence needing to report), or getting to the max interval (likewise), or CanBeSynced being true and another ReadHandler needing to report, in which case we might as well also report.
Do those descriptions make sense? I think recasting this documentation in terms sort of like those would be a lot clearer....
| * This flag can substitute the maximal interval condition or the dirty condition. It is currently only used by the | ||
| * SynchronizedReportScheduler. | ||
| * | ||
| * EngineRunScheduled: Mechanism to ensure that the reporting engine will see the ReadHandler as reportable if a timer fires. |
There was a problem hiding this comment.
Bringing us back to: why is this called EngineRunScheduled? Is that the really important thing? Or is the important thing "we think we have reached our min interval, even though the clock disagrees"?
There was a problem hiding this comment.
It means this: "This ReadHandler's timer fired, thus ScheduleReport was called, and we want the Report Engine to see the ReadHandler as reportable on the next run, regardless of what the clock thinks"
This could be renamed "TimerFired" flag if this seems more clear. Will update the description in my ongoing re-write, I can also update the name if you think it would make sense.
There was a problem hiding this comment.
How about ForceReportableOnNextEngineRun if that's what it means?
| * | ||
| * EngineRunScheduled: Mechanism to ensure that the reporting engine will see the ReadHandler as reportable if a timer fires. | ||
| * This flag can substitute the minimal interval condition or the maximal interval condition. The goal is to allow for | ||
| * reporting when timers fire earlier than the minimal timestamp du to mechanism such as NTP clock adjustments. |
There was a problem hiding this comment.
s/du/due/, but this whole comment might end up changing a bunch.
| * This class implements a scheduling logic that calculates the next report timeout based on the current system timestamp, the state | ||
| * of the ReadHandlers associated with the scheduler nodes and the min and max intervals of the ReadHandlers. | ||
| * | ||
| * @note This class mimics the original scheduling in which the ReadHandlers would schedule themselves. The key difference is that |
There was a problem hiding this comment.
I don't think anyone cares that this "mimics" code that is now gone. It would be better instead to clearly describe the algorithm this class is aiming to implement.
| * While looping, it checks if any handler is reportable now. If not, we recalculate the next report timeout and reschedule the | ||
| * report. | ||
| * | ||
| * If a Readhangler is reportable now, an engine run is scheduled. |
There was a problem hiding this comment.
What is a "Readhangler"?
| * | ||
| * If a report is already scheduled, cancel it and schedule a new one. | ||
| * | ||
| * @param[in] timeout The timeout to schedule the report. |
There was a problem hiding this comment.
The delay before the report will happen, no?
Comment on lines
+154
to
+155
| * @note Since this method is called after the OnSubscriptionReportSent callback, to avoid an endless reporting loop, Nodes with | ||
| * the IsEngineRunScheduled flag set are ignored when finding if the Scheduler should report at min, max or now. |
There was a problem hiding this comment.
This is much better as comments go, but I wish it explained why not ignoring them would lead to "an endless reporting loop"....
There was a problem hiding this comment.
Loop description added.
| * @note Since this method is called after the OnSubscriptionReportSent callback, to avoid an endless reporting loop, Nodes with | ||
| * the IsEngineRunScheduled flag set are ignored when finding if the Scheduler should report at min, max or now. | ||
| * | ||
| * @note If a ReadHandler's report is Chunked, the IsEngineRunScheduled is ignored since we do want to keep rescheduling the |
There was a problem hiding this comment.
"chunked" is not a proper noun.
| * @note Since this method is called after the OnSubscriptionReportSent callback, to avoid an endless reporting loop, Nodes with | ||
| * the IsEngineRunScheduled flag set are ignored when finding if the Scheduler should report at min, max or now. | ||
| * | ||
| * @note If a ReadHandler's report is Chunked, the IsEngineRunScheduled is ignored since we do want to keep rescheduling the |
There was a problem hiding this comment.
This part of the documentation is confusing. Is this discussing an exception to the "avoid an endless loop" bit above? If so, it would be clearer to just describe what situations we want to reschedule in and what situations not, without this weird "we do X, except no, in case Y we do Z" structure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR clarifies the logic behind the ReportScheduler base class and its two implementation:
ReportSchedulerImpl and SynchronizedReportSchedulerImpl